[cxx-interop] Remove annotationOnly flag from Escapability request #85643
Conversation
susmonteiro
requested review from
Xazax-hun,
beccadax,
hnrklssn,
ian-twilightcoder and
j-hui
as code owners
susmonteiro
changed the title
|
@swift-ci please smoke test |
| type->isMemberPointerType() || type->isReferenceType()) { | ||
| // pointer and reference types are currently imported as unknown | ||
| // (importing them as non-escapable broke backward compatibility) | ||
| hasUnknown = true; |
There was a problem hiding this comment.
I'm fine with this for now, to preserve the current behavior, but assuming pointers have unknown escapability seems like it could be problematic. My understanding is that, semantically, pointers should be treated as non-escapable.
Do we have a plan for how to correctly analyze these types while maintaining backwards compat?
An idea I had was making the analysis depend on whether strict memory safety is turned on or off, essentially using/abusing it as a feature flag. But that may not be a good idea for reasons that I haven't fully thought through yet.
| void someMethod() {} | ||
| }; | ||
|
|
||
| // This is a complex record, so we don't infer escapability. |
There was a problem hiding this comment.
Can you leave a reminder about what makes this record complex? Is it the presence of a user-defined default constructor? Or copy constructor?
| View b; | ||
| bool c; | ||
|
|
||
| ComplexRecord() {} |
There was a problem hiding this comment.
nit: please initialize the fields
| // as such | ||
| // - a record type is escapable if it is annotated with SWIFT_ESCAPABLE_IF() | ||
| // and none of the annotation arguments are non-escapable | ||
| // - an aggregate or a record that is not a cxxrecord are escapable if none of |
There was a problem hiding this comment.
nit: simplify wording
| // - an aggregate or a record that is not a cxxrecord are escapable if none of | |
| // - an aggregate or non-cxx record is escapable if none of |
I think it might also be helpful to spell out what it means to be an "aggregate" record. In my mind I read this as basically being "a C-like record", i.e., not having user-defined behavior for copy construction, but maybe that's inaccurate/incomplete.
Also, can you look up/document the reason why we are distinguish aggregate records from non-aggregate/"complex" records?
There was a problem hiding this comment.
Oh, I see that the reasoning is sort of written about in a later comment. I think that should be brought up here, to keep things in one place.
2 participants
The
annotationOnlyflag was originally introduced to control for which types we would infer escapability, and for which we would rely solely on annotations. However, due to backward compatibility issues, this flag is currently always set totrue. Therefore, this patch removes theannotationOnlyflag and makes the compiler infer escapability from bases and fields only for simple records, such as aggregates.